Fix module check nested validity - #2913

Merging to main in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

Automated review by Claude. No issues found.

This is the largest of the module-validation PRs and it holds up on close reading:

• #[serde(deny_unknown_fields)] on ModuleHeader catches header typos (e.g. inputz), covered by check_module_file_rejects_unknown_module_header_field.
• check_module_file becomes recursive via check_module_file_inner with a depth limit and a seen set. The success path calls seen.remove(canonical), so diamond dependencies (the same module referenced by two siblings) are still accepted while genuine cycles are rejected — a nice distinction. The depth test drives a linear chain of MAX_MODULE_DEPTH+1 and reaches the bail correctly.
• Declared outputs are tracked per-producer (BTreeMap<String, Vec<String>>), so an ambiguous declared output (>1 inner producer) is now rejected instead of silently resolving to the first match — enforced in both check_module_file and expand_module_node (direct_output_targets).
• Nested-module output boundaries are enforced: expand_module_node builds direct_output_targets from each nested module's declared output map (nested_omap) rather than scanning fully-expanded leaf nodes, so a parent can no longer re-export a nested module's private (non-declared) output. Real behavior fix, covered by expand_rejects_nested_module_private_output_export.
• check_inner_node_output_refs / check_nested_module_output_refs validate that inner input references point to outputs actually declared by the referenced sibling node / nested module. These stay consistent with node_output_refs (operator outputs addressed as op_id/output, others bare), so they don't false-positive on valid multi-operator references, and _mod/ / unknown sources are safely skipped.

Tests are meaningful and cover each new rejection path. One minor, non-blocking observation: check_module_file_inner calls load_module_file(&nested_canonical) to collect outputs and then recurses (which loads it again) — a redundant parse, not a correctness problem.

🤖 This is a fully automated review by Claude (Claude Code). No human has vetted these findings — please verify before acting on them.

Generated by Claude Code

Generated by Claude Code

🤖 Automated review by Claude — this comment is fully automated and has not been vetted by a human.

No issues in the diff itself — the ambiguous-output detection and the nested private-output boundary check look correct, and the added tests assert specific error substrings and would fail if the checks were removed.

One cross-PR hazard worth flagging, since this overlaps several sibling PRs editing the same function: this PR moves the body of check_module_file into the new check_module_file_inner, which loads the module file but does not call validate_module_header (added by #2907) or check_nested_module_required_inputs (added by #2912). Both of those PRs insert their calls into the old check_module_file at exactly this region. Whichever lands second will conflict here, and if this rewrite is taken as-is, those validations are dropped from the check_module_file path (the required/optional-input-conflict check and the nested-required-input check). Whoever merges should re-add those calls into check_module_file_inner and the nested-recursion site so the checks aren't lost.

More broadly, this and the other module PRs in the #2901–#2913 range all edit check_module_file / expand_module_node / substitute_params_in_node from the same base and won't apply as a clean independent stack — they'd be safer landed as one coordinated series with the overlaps reconciled.

Generated by Claude Code

docs/modules.md still documents the old rule. Its wiring checks duplicate #2903's, which are better — drop them. Also deny_unknown_fields on ModuleHeader applies to dora run, not just the linter.

🤖 Automated review by Claude — fully automated, no human reviewed this; please verify before acting.

The commits since my last review (docs alignment in docs/modules.md, plus the main merge and its formatting repair) look safe and address the outstanding points:

• The cross-PR hazard I flagged earlier did not materialize: check_module_file_inner in libraries/core/src/descriptor/expand.rs still calls validate_module_header before recursing, so the required/optional-input conflict check is preserved through the merge.
• The duplicate wiring checks are gone from the final diff (no check_inner_node_output_refs / check_nested_module_output_refs), leaving fix(core): validate module file internal wiring #2903's checks as the single source.
• docs/modules.md now matches the implemented rules (direct-child / nested-declared outputs, ambiguity rejection, and deny_unknown_fields applying to dora run / dora build, not just the linter).

No new issues in the diff.

Generated by Claude Code

🤖 This is a fully automated review by Claude. No human has verified these findings.

Re the earlier "cross-PR hazard" note — I don't think it's fully resolved, and I believe the most recent "did not materialize" conclusion checked the wrong function.

#2912 (merged to main ~11:21 UTC on 2026-08-13) added a check_nested_module_required_inputs call inside check_module_file, so the standalone linter rejects an outer module that fails to wire a nested module's required inputs. This branch merged main in earlier (~08:07 UTC), before #2912 landed, and rewrote the body into check_module_file_inner (libraries/core/src/descriptor/expand.rs:171), which calls validate_module_header but not check_nested_module_required_inputs (that function doesn't exist on this branch).

Consequences:

• Merging into current main will conflict in check_module_file; taking this rewrite as-is silently drops fix(core): validate nested module required inputs #2912's nested-required-input validation.
• Even setting the merge aside, it leaves a check-vs-run divergence: expand_module_node still enforces required inputs for module nodes (expand.rs:502), so dora run rejects a missing nested required input while dora expand --module would accept it — the same class of inconsistency this PR aims to close.

The prior "did not materialize" conclusion appears to have verified validate_module_header (the required/optional-input conflict check within a single header) rather than check_nested_module_required_inputs (#2912's distinct check that the parent provides a nested module's required inputs) — these are separate functions.

Suggest rebasing on current main and re-adding the check_nested_module_required_inputs call into check_module_file_inner's nested-module loop (~expand.rs:262).

Separately: the recursion/cycle/depth logic and the new ambiguity + private-output-boundary checks look correct and are well-tested.

Generated by Claude Code